Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Revert default mesh materials #15930

Merged
merged 1 commit into from
Oct 15, 2024
Merged

Revert default mesh materials #15930

merged 1 commit into from
Oct 15, 2024
+18 −178

Conversation

Jondolf
Copy link
Contributor

@Jondolf Jondolf commented Oct 15, 2024 •
edited
Loading

Objective

Closes #15799.

Many rendering people and maintainers are in favor of reverting default mesh materials added in #15524, especially as the migration to required component is already large and heavily breaking.

Solution

Revert default mesh materials, and adjust docs accordingly.

  • Remove extract_default_materials
  • Remove clear_material_instances, and move the logic back into extract_mesh_materials
  • Remove HasMaterial2d and HasMaterial3d
  • Change default material handles back to pink instead of white
    • 2D uses Color::srgb(1.0, 0.0, 1.0), while 3D uses Color::srgb(1.0, 0.0, 0.5). Not sure if this is intended.

There is now no indication at all about missing materials for Mesh2d and Mesh3d. Having a mesh without a material renders nothing.

Testing

I ran 2d_shapes, mesh2d_manual, and 3d_shapes, with and without mesh material components.

@Jondolf Jondolf added A-Rendering Drawing game state to the screen X-Blessed Has a large architectural impact or tradeoffs, but the design has been endorsed by decision makers D-Straightforward Simple bug fixes and API improvements, docs, test and examples labels Oct 15, 2024
@alice-i-cecile alice-i-cecile added C-Usability A targeted quality-of-life change that makes Bevy easier to use and removed X-Blessed Has a large architectural impact or tradeoffs, but the design has been endorsed by decision makers D-Straightforward Simple bug fixes and API improvements, docs, test and examples labels Oct 15, 2024
@alice-i-cecile alice-i-cecile added this to the 0.15 milestone Oct 15, 2024
@alice-i-cecile alice-i-cecile added S-Needs-Review Needs reviewer attention (from anyone!) to move forward D-Domain-Agnostic Can be tackled by anyone with generic programming or Rust skills labels Oct 15, 2024
@Jondolf Jondolf added X-Blessed Has a large architectural impact or tradeoffs, but the design has been endorsed by decision makers D-Straightforward Simple bug fixes and API improvements, docs, test and examples labels Oct 15, 2024
ecoskey
ecoskey approved these changes Oct 15, 2024
View reviewed changes
Copy link
Contributor

@ecoskey ecoskey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have we decided on not needing/wanting a debug warning at least? Otherwise LGTM

@Jondolf
Copy link
Contributor Author

Jondolf commented Oct 15, 2024

A warning for missing materials would probably require adding HasMaterial2d/HasMaterial3d again, and my understanding was that we want to remove it. And custom rendering solutions (which I believe were one reason for wanting to remove default materials) would then also need to deal with the warnings.

@Jondolf Jondolf added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Oct 15, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Oct 15, 2024
Merged via the queue into bevyengine:main with commit c1a4b82 Oct 15, 2024
38 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tychedelia tychedelia tychedelia approved these changes

@ecoskey ecoskey ecoskey approved these changes

Assignees
No one assigned
Labels
A-Rendering Drawing game state to the screen C-Usability A targeted quality-of-life change that makes Bevy easier to use D-Domain-Agnostic Can be tackled by anyone with generic programming or Rust skills D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it X-Blessed Has a large architectural impact or tradeoffs, but the design has been endorsed by decision makers
Projects
Rendering
Status: Done
Milestone
0.15
Development

Successfully merging this pull request may close these issues.

Revert default mesh material
4 participants
@Jondolf @tychedelia @ecoskey @alice-i-cecile